Support array for serviceEndpoint #1200
Conversation
nikolockenvitz
requested review from
thehenrytsai,
tplooker,
csuwildcat,
troyronda,
OR13 and
xinaxu
as code owners
|
@thehenrytsai looks like a good one to pull in, given the DID Comms enablement |
|
@nikolockenvitz I would add the breaking change to the commit, yes |
| @@ -4,5 +4,5 @@ | |||
| export default interface ServiceModel { | |||
| id: string; | |||
| type: string; | |||
| serviceEndpoint: string | object; | |||
| serviceEndpoint: string | object | Array<string | object>; | |||
There was a problem hiding this comment.
Probably no need for this change, array is an object.
There was a problem hiding this comment.
That's true, I just thought it might be better to understand if it's explicit. But I can also revert this change if you like.
| for (const serviceEndpoint of serviceEndpointValueAsArray) { | ||
| // serviceEndpoint itself must be URI string or non-array object | ||
| if (typeof serviceEndpoint === 'string') { | ||
| const uri = URI.parse(serviceEndpoint); | ||
| if (uri.error !== undefined) { | ||
| throw new SidetreeError( | ||
| ErrorCode.DocumentComposerPatchServiceEndpointStringNotValidUri, | ||
| `Service endpoint string '${serviceEndpoint}' is not a valid URI.` | ||
| ); | ||
| } | ||
| } else if (typeof serviceEndpoint === 'object') { | ||
| // Allow `object` type only if it is not an array. | ||
| if (Array.isArray(serviceEndpoint)) { | ||
| throw new SidetreeError(ErrorCode.DocumentComposerPatchServiceEndpointCannotBeAnArray); | ||
| } | ||
| } else { | ||
| throw new SidetreeError(ErrorCode.DocumentComposerPatchServiceEndpointMustBeStringOrNonArrayObject); |
There was a problem hiding this comment.
We will need the same change in ion-sdk. Totally get it if you don't have time to make those changes in ion-sdk, so the PR will suffice to be approved if you can file an issue in the ion-sdk repo to make the same changes! Thanks!
There was a problem hiding this comment.
Happy to implement this in ion-sdk as well. I will still create an issue as well, as I am not yet 100% sure when I will find time for the implementation. So I will open the issue, once this PR is merged, right?
There was a problem hiding this comment.
Hi @thehenrytsai, I updated ion-sdk (decentralized-identity/ion-sdk#30) as well
nikolockenvitz
dismissed stale reviews from BenjaminMoe, troyronda, and OR13
via
4012c5f
|
@thehenrytsai can you please check whether all your concerns are addressed or point out required changes? |
related to #1198 @csuwildcat
should the breaking change notice be part of the merge commit?